feat: add durable-functions integration#7535
Conversation
Overall package sizeSelf size: 4.79 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.6 | 81.92 kB | 816.75 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #7535 +/- ##
==========================================
- Coverage 80.30% 80.21% -0.09%
==========================================
Files 733 735 +2
Lines 31565 31608 +43
==========================================
+ Hits 25348 25355 +7
- Misses 6217 6253 +36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38dad4ce27
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return function (activityName, activityOptions) { | ||
| shimmer.wrap(activityOptions, 'handler', handler => { | ||
| const isAsync = | ||
| handler && handler.constructor && handler.constructor.name === 'AsyncFunction' |
There was a problem hiding this comment.
The activity wrapper chooses traceSync unless handler.constructor.name === 'AsyncFunction', which misses valid handlers that return a Promise but are not declared with async (for example transpiled async functions or plain functions returning Promise). In that case the span is finished on end before the Promise settles, so latency and rejection errors are not captured for the activity execution.
Useful? React with 👍 / 👎.
| type: 'serverless', | ||
|
|
||
| meta: { | ||
| component: 'azure-functions', |
There was a problem hiding this comment.
The plugin starts spans for azure-durable-functions but explicitly sets meta.component to azure-functions, overriding the plugin component and labeling these spans as the wrong integration. This causes durable-function traces to be attributed to the Azure Functions integration instead of their own component.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Yeah, are we considering these to be the same component as standard azure functions or should we make this have a more specific component tag for durable functions?
BenchmarksBenchmark execution time: 2026-02-24 15:07:10 Comparing candidate commit 32d5a54 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 230 metrics, 30 unstable metrics. |
ojproductions
requested review from
khanayan123
and removed request for
a team
| addHook({ name: '@azure/functions', versions: ['>=4'], patchDefault: false }, (azureFunction) => { | ||
| const { app } = azureFunction | ||
|
|
||
| if (alreadyPatched) return azureFunction |
There was a problem hiding this comment.
for some reason, when using esm imports, azure functions gets patched twice, resulting in extra spans. hence I added this. Why does this happen?
There was a problem hiding this comment.
For ESM we always patch both the default export and the outer export.
In some cases, the default export and the outer export end up triggering a patch on the same object or function. For example, we observed this behavior when a patch was applied to the prototype of an object that was shared by both the default export and the outer export.
There was a problem hiding this comment.
does the patchDefault: false option in addhook not affect this behavior at all?
There was a problem hiding this comment.
I solved this in service bus by using a weakmap like this. I'm not sure if that's the approach we want to take here though.
There was a problem hiding this comment.
Me and @pabloerhard talked and it seems this may be due to how durable-functions is importing @azure/functions under the hood using require. He's working on a fix in #7601
jcstorms1
left a comment
jcstorms1
left a comment
There was a problem hiding this comment.
Looks good overall. Left a few questions and a couple suggestions. Nice job!
| type: 'serverless', | ||
|
|
||
| meta: { | ||
| component: 'azure-functions', |
There was a problem hiding this comment.
Yeah, are we considering these to be the same component as standard azure functions or should we make this have a more specific component tag for durable functions?
| super.finish(ctx) | ||
| } | ||
|
|
||
| asyncEnd (ctx) { |
There was a problem hiding this comment.
I know it seems strange, but it is better to make this asyncStart for a more accurate trace. My initial intuition was to use these to capture the end of traceAsync, but I was corrected as well.
asyncStart and asyncEnd are essentially occurring right after each other without anything happening in-between. In fact, asyncEnd isn't necessary in the majority of cases. The edge case would be if we need information from a call back function, which we do not patch in this integration.
start
code patched
end
asyncStart
if there was a callback it would execute here.
asyncEnd
| kind: 'internal', | ||
| type: 'serverless', | ||
|
|
||
| meta: { |
There was a problem hiding this comment.
I'm not sure if you have this in a doc or not, but I'd double check we have all the metadata we need and compare it with a normal function. I imagine a lot of the Azure tags won't be necessary for durable functions, but we should make sure there is as much in common as possible.
| await agent.stop() | ||
| }) | ||
|
|
||
| it('is instrumented', async () => { |
There was a problem hiding this comment.
Do we need a separate test for entity and non-entity functions or is this sufficient? I believe there is separate logic in the case of an entity function and we should probably capture that.
| async function spawnPluginIntegrationTestProc (cwd, command, args, agentPort, stdioHandler, additionalEnvArgs = {}) { | ||
| let env = { | ||
| NODE_OPTIONS: `--loader=${hookFile} func start`, | ||
| NODE_OPTIONS: `--loader=${hookFile}`, |
There was a problem hiding this comment.
How does func start initialize now? I may have missed it.
ojproductions
changed the title
3 participants
What does this PR do?
Adds auto instrumentation for azure durable-functions package
Motivation
APMSVLS-332
SVLS-7644
Additional Notes